fix(ledmatrix-music): scroll long now-playing text instead of overflowing (+ GPL-3.0)#179
Conversation
…low) + GPL-3.0 The now-playing title/artist/album marquee drew the full (rotated) string at the text-area x with no right-edge clip, so long text ran past the panel's right edge. The safety harness flagged this as an overflow on the 64px-wide panels (64x32 / 64x64), where the text area is only ~29px wide. Add _clip_text_to_width() and apply it to all four text draws so the marquee stays inside its viewport: long text still scrolls (the char-based rotation is unchanged) — it's just clipped on the right instead of overflowing the panel. Also relicense to GPL-3.0 (LICENSE + manifest + README). music was the plugin deferred from the repo-wide MIT -> GPL-3.0 pass because of this overflow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a text-clipping helper to the ledmatrix-music plugin's display() method to prevent title, artist, and album text from overflowing the drawable width, tightens the progress-bar render guard, and changes the plugin's license from MIT to GPLv3 with corresponding manifest, README, LICENSE, and registry metadata updates. ChangesDisplay Clipping and License Update
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Display as display()
participant Clip as _clip_text_to_width
participant DM as display_manager
Display->>Clip: clip title/artist/album text, text_area_width
Clip->>DM: get_text_width(text, font)
DM-->>Clip: measured text width
Clip-->>Display: clipped text (or empty string)
Display->>Display: draw clipped text within bounds
Display->>Display: check text_area_width > 0 before drawing progress bar
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 13 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…quare panels) Follow-up to the viewport clip: on square panels (e.g. 64x64) the album art is sized to the panel height, so it fills the full width and text_area_width goes negative. The clip helper's `max_width <= 0` guard returned the text UNCLIPPED, so it was still drawn off-panel (overflow bbox=(64,28,91,36) at 64x64). Now return an empty string when there's no room, and skip the progress bar too. Album art fills the panel; no text is drawn rather than overflowing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
…width That was the real overflow the harness caught (CI has no auth -> no track -> the "Nothing Playing" idle screen renders). The text is centered via x = (width - text_width)//2, which goes negative when the text is wider than the panel (64px), spilling off both edges — matching the failing bbox exactly (y = height//2 - 4 = 12 at 64x32, 28 at 64x64). Clip the text to the panel width and clamp x to >= 0. (The earlier marquee viewport clip is still correct for the now-playing view with a real track.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ledmatrix-music/manager.py (1)
1068-1072: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"..."truncation indicator may be clipped away in the scrolling-disabled path.The rough truncation
title[:text_area_width // 6] + "..."produces a string that is typically wider thantext_area_width, so_clip_text_to_width(applied at line 1081) will trim further—often removing the"..."entirely. The same pattern exists for artist (line 1125) and album (line 1199). Users who explicitly disable scrolling lose the visual cue that text was truncated.Consider either dropping the rough truncation and relying solely on
_clip_text_to_width, or clipping first and appending"..."only if clipping actually occurred.💡 Suggested approach
elif title_width > text_area_width and not title_config['enabled']: - # Scrolling disabled - truncate text - current_title_display_text = title[:text_area_width // 6] + "..." # Rough truncation + # Scrolling disabled - clip to viewport, add ellipsis if truncated + clipped = self._clip_text_to_width(title, font_title, text_area_width) + current_title_display_text = clipped + ("..." if clipped != title else "") self.scroll_position_title = 0 self.title_scroll_tick = 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ledmatrix-music/manager.py` around lines 1068 - 1072, The scrolling-disabled truncation paths in the title, artist, and album display logic are appending "..." before width clipping, which can cause the indicator to be clipped away. Update the display flow in the title/artist/album handling blocks so that truncation is handled either entirely by _clip_text_to_width or by appending "..." only after confirming the text was actually clipped, and preserve the indicator when scrolling is disabled.
🧹 Nitpick comments (1)
plugins/ledmatrix-music/manager.py (1)
736-757: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
_clip_text_to_widthlogic is correct and well-documented.The helper handles all edge cases properly:
max_width <= 0returns"", falsy text is returned as-is, already-fitting text is returned unchanged, and the trim loop correctly degenerates to""when even a single character exceedsmax_width.One minor performance note: the while-loop calls
get_text_widthonce per removed character, making it O(n²) in the worst case for the underlying font measurement. For typical music titles on small panels this is fine, but a binary search on the trim length would reduce it to O(n log n) if profiling ever shows hot spots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ledmatrix-music/manager.py` around lines 736 - 757, The `_clip_text_to_width` helper in `manager.py` is correct but the trimming loop is O(n²) because it measures text width after removing one character at a time. Update `_clip_text_to_width` to use a more efficient search strategy, such as binary search on the clipped length with `self.display_manager.get_text_width(...)`, while preserving the existing edge cases for empty text and non-positive `max_width`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugins/ledmatrix-music/manager.py`:
- Around line 1068-1072: The scrolling-disabled truncation paths in the title,
artist, and album display logic are appending "..." before width clipping, which
can cause the indicator to be clipped away. Update the display flow in the
title/artist/album handling blocks so that truncation is handled either entirely
by _clip_text_to_width or by appending "..." only after confirming the text was
actually clipped, and preserve the indicator when scrolling is disabled.
---
Nitpick comments:
In `@plugins/ledmatrix-music/manager.py`:
- Around line 736-757: The `_clip_text_to_width` helper in `manager.py` is
correct but the trimming loop is O(n²) because it measures text width after
removing one character at a time. Update `_clip_text_to_width` to use a more
efficient search strategy, such as binary search on the clipped length with
`self.display_manager.get_text_width(...)`, while preserving the existing edge
cases for empty text and non-positive `max_width`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 858a4e64-b2ce-48c9-b79a-868356d7db8e
📒 Files selected for processing (5)
plugins.jsonplugins/ledmatrix-music/LICENSEplugins/ledmatrix-music/README.mdplugins/ledmatrix-music/manager.pyplugins/ledmatrix-music/manifest.json
… text When scrolling is disabled, the title/artist/album were truncated with a rough char-count estimate (`text[:width//6] + "..."`) and then passed through the draw-time viewport clip, which could trim the trailing "..." back off — leaving no truncation indicator. Add `_truncate_text_with_ellipsis`, which reserves room for the ellipsis before clipping so the "..." always survives and the result fits the text-area viewport. Bumps ledmatrix-music to 1.0.8. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
Follows the maintainer's correction on #175: the core's manifest validator flags `ledmatrix_min` as deprecated and expects `ledmatrix_min_version`. Switch the music versions[] entries (1.0.8, 1.0.7, 1.0.6) to `ledmatrix_min_version` so device installs don't log a deprecated-field warning. Also merges the latest main (#174, #175) and regenerates plugins.json. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
2 participants
Summary
Fixes the now-playing text overflowing the right edge on narrow (64px-wide) panels, and completes the repo-wide MIT→GPL-3.0 pass (music was the one plugin deferred because of this bug).
The issue
Album art is a square of the panel height, so on a 64×32 panel it's 32px wide and the text area is only ~29px (x 34–63). The title/artist/album marquee builds a rotated string (
text[pos:] + sep + text[:pos]) and draws the whole thing at the text-area x with no right-edge clip. The character-based scroll changes which characters lead, but the drawn string is still full-length, so long text always runs off the right edge (and during the initial "pause" it draws the untrimmed title). The safety harness caught this asnow_playing overflow bbox=(64,…)at 64×32 and 64×64.The fix
Add
_clip_text_to_width(text, font, max_width)and apply it to all four text draws (title, artist, album-fits, album-scroll). It trims trailing characters until the rendered text fits the text-area width, so the marquee stays inside its viewport. Long text still scrolls — the char-based rotation is unchanged; it's just clipped on the right instead of drawing past the panel edge. No behavior change on wide panels where text already fit.Plugin(s) affected
ledmatrix-music(1.0.6 → 1.0.7)Type of change
Test plan
python -c "import ast; ast.parse(...)"(parses),update_registry.py(1.0.6→1.0.7),check_module_collisions.py(clean), JSON validity.ledmatrix-musicat all sizes — this is the authoritative check for the overflow fix (the harness is what caught it; I can't run it locally). If any size still overflows I'll iterate.Context
Part of the maintainer-requested "no MIT, all GPL-3.0" pass. With this merged, every plugin is GPL-3.0. (Companion license PRs: #173, #175, #177.)
🤖 Generated with Claude Code
https://claude.ai/code/session_01F9Saiat9CQmNi3DeDdvacB
Generated by Claude Code
Summary by CodeRabbit
New Features
ledmatrix-musicplugin to version 1.0.7.Bug Fixes
Documentation